fix: async subscription do not run when mutate state synchronously after patch #2870
Conversation
✅ Deploy Preview for pinia-official canceled.
|
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks! I will take a look when I can |
yangmingshan
force-pushed
the
subscribe
branch
from
60b9c6d to
7844dba
Compare
|
This PR is now based on Does |
|
Thanks! I think it's fine to keep this for v3 only so don't worry about it |
commit: |
| () => { | ||
| shouldTrigger = isListening | ||
| }, | ||
| { deep: true, flush: 'sync' } |
There was a problem hiding this comment.
I'm revisitting this PR and realizing that this might impact perf too much
There was a problem hiding this comment.
I need to see in the playground the impact
There was a problem hiding this comment.
Without checking the playground, I think we can't go this way: #610
WalkthroughRefactors store subscription internals to replace multi-flag listening with a single gating flag and two coordinated watchers, and updates tests (adjusted expectations and one new test) reflecting changed sync flush behavior and debugger event shape for non-patch subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(173,216,230,0.15)
participant User
participant Store
participant WatcherFull as Watcher (full state)
participant WatcherDirect as Watcher (direct/patch)
participant Subscribers
end
User->>Store: apply patch / HMR payload
Store->>Store: isListening = false\nshouldTrigger = true
Store->>WatcherFull: full-state watcher fires
WatcherFull->>Store: checks shouldTrigger
alt shouldTrigger true
WatcherFull->>Subscribers: notify (patch events)
end
Store->>Store: isListening = true
User->>Store: perform direct mutation
Store->>Store: isListening = false\nshouldTrigger = true
Store->>WatcherDirect: direct-change watcher fires
WatcherDirect->>Store: checks shouldTrigger
alt shouldTrigger true
WatcherDirect->>Subscribers: notify (direct-change event)
end
Store->>Store: isListening = true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/pinia/__tests__/subscriptions.spec.ts (1)
357-357: Fix grammatical error in test description.The test name contains a grammatical error: "not be an array" should be "not an array".
Apply this diff:
- it('debuggerEvents is not be an array when subscription is not trigger by patch', () => { + it('debuggerEvents is not an array when subscription is not triggered by patch', () => {packages/pinia/src/store.ts (1)
267-268: Consider clarifying the shouldTrigger initialization.While the comment states "The initial value does not matter", initializing
shouldTriggertoundefinedmay cause confusion during debugging. Consider either:
- Initializing to
falsefor clarity, or- Expanding the comment to explain that it's set synchronously before being checked by async watchers
Apply this diff:
- let shouldTrigger: boolean | undefined // The initial value does not matter, and no need to set to true at the end + // Captures the listening state synchronously for async watchers; set by sync watcher before async watchers check it + let shouldTrigger: boolean | undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/pinia/__tests__/subscriptions.spec.ts(1 hunks)packages/pinia/src/store.ts(7 hunks)
🔇 Additional comments (5)
packages/pinia/__tests__/subscriptions.spec.ts (1)
371-385: Good test coverage for the fix.This test effectively validates the core fix for issue #992, ensuring that subscriptions with all flush strategies ('sync', 'pre', 'post') are properly triggered when state is synchronously mutated after a patch operation.
packages/pinia/src/store.ts (4)
252-252: Condition refinement looks good.The added
!store._hotUpdatingcheck appropriately prevents debugger event accumulation during hot module replacement operations.
291-291: isListening state management is correct.The bookending of the patch operation with
isListening = false/trueproperly prevents automatic watcher firing during the patch, while manual triggering (lines 315-319) ensures subscribers are notified once.Also applies to: 313-313
624-624: HMR state transition is consistent.The synchronous
isListening = trueassignment aligns with the removal of nextTick-based state transitions and maintains consistency with the$patchimplementation.
437-467: Dual-watcher design appears intentional; performance concern is valid but unproven.Analysis reveals this is a deliberate implementation to handle timing correctly: during state mutations,
shouldTriggercaptures the currentisListeningstate synchronously viastop1, thenstop2fires the callback only if that captured state permits it. The design prevents subscriptions from firing at incorrect times due to race conditions with theisListeningflag.The performance overhead concern is valid—two watchers per subscription with deep watching on every mutation—but lacks empirical measurement. The codebase contains no performance benchmarks or complaints about subscription overhead. The trade-off appears accepted for correctness.
Before accepting or modifying this approach, measure actual performance impact in your usage patterns. If performance is acceptable, no changes needed. If degradation is significant, consider profiling to identify if this dual-watcher is the bottleneck or if other optimizations would suffice.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3 #2870 +/- ##
==========================================
- Coverage 90.90% 90.90% -0.01%
==========================================
Files 19 19
Lines 1683 1682 -1
Branches 237 235 -2
==========================================
- Hits 1530 1529 -1
Misses 151 151
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yangmingshan
force-pushed
the
subscribe
branch
from
bebea16 to
807cfd6
Compare
2 participants
Fix #992
I run into same issue when I porting Pinia to Vue Mini, and I found another debugger issue for sync subscription (please see the test).
I managed a way to fix them, the fix may have extra memory / performance costs, I didn't do any measurements. But I used it in Pinia for Vue Mini anyway.
Summary by CodeRabbit
Tests
Improvements